LGTM, tested and works

Hello @manchoz I'm struggling to understand why there is the need to disabled SNI.

I do understand the need for self-signed certificates and home-made PKI (and we need to improve that), but AFAIU disabling SNI can pose a serious security risk (no protection for MITM) even in local networks and is not necessary to setup a home-made PKI.

Can you help me understand?

Ciao @endorama was add only to support a customer that need to connect a MKR1000 to his local mqtt broker using SSL/TLS, and was achieved by disabling the SNI, if you think that is to much dangerous we revert the commit.

Hi @endorama,
its mainly intended to reproduce the --insecure option of some popular tools such as curl or mosquitto_sub/pub to help the users test their home-made PKI and self-signed certificates when Certificate Signing Requests and Certificates are not setup with the proper subjectAltNames field (which is the root cause in the 99% of the help requests).

Currently, the parameter is false by default an I think we can make an effort to render the security risk more explicit, eg. renaming the parameter or letting the parameter accept an enum such as CONNECTION_INSECURE or similar. We might also remove the parameter from the constructor and add a ::setInsecureMode() method.

Please, note that the original code already allows insecure connections when using an IPAddress-type address for connection.

I totally agree with you that we need to produce a detailed guide (on par with the Arduino Cloud Provider Examples series) to explain how to properly setup an at-home PKI and how-to generate CSRs and certificates with proper SNI support and how to use it with the Arduino boards (generating the CSR, generating the CRT, uploading the CRT and create the ad-hoc Trusted Anchor).

What do you think?

That option has security implications that are dangerous, is not dangerous per se (as in "giving a gun to a child"). From an Arduino POV, this option used unwisely reduces (if not completely eliminates) any security provided by TLS (in particular for over the internet connections).

are not setup with the proper subjectAltNames field (which is the root cause in the 99% of the help requests)

This is the exact case in which I fear such a field would be used. If we are suggesting to disable SNI checks as a workaround we are doing it wrong 😓

To wrap up my considerations:

• design wise I would go for the ::setInsecureMode() method (so no ambiguity is present when creating a new object instance). Adding detailed documentation to such a method is probably enough to explain and alert on security issues
• is dealing with improperly configured brokers something we want to support?
• we need to provide proper guidance on setup appropriate security, as we support it! This is a better way to help diverge support requests than providing such a workaround

Please, note that the original code already allows insecure connections when using an IPAddress-type address for connection.

As is reasonable to expect usage of TLS with IP address, may you help me understand the logic flow that happens in such a case? Having TLS certificate issued for IP addresses in relatively uncommon, but supported for public IPs and there is no issuing issue for private PKIs.
Is insecure method selected by default if an IP is specified?

@Rocketct @manchoz what do you think? Can we at least implement the ::setInsecureMode() quickly to buy us time to go in deep in identifying other solutions or workarounds?

@endorama Yes, I will make a new PR ASAP.

Getting back to @endorama ... When connecting via a NB-IoT network via MKR NB 1500 then you even have to disable SNI because NB-IoT networks don't provide DNS hence you have to no host name either ( arduino-libraries/ArduinoIoTCloud@b464613 ). An alternative could be to provide a lookup-table (host name for IP) in order to prevent using no SNI when connecting via IP only. Not sure if this will work though.

@aentinger from my understanding of SNI, was born to allow requesting the proper server certificate for HTTP connections: as TLS handshake happens before HTTP it was not possible to have 1 single listeners on a server serving multiple certificates. SNI allows the client to request, during ClientHello to request the server certificate it wants.
If this is true the same may happen with MQTT, where TLS happens before MQTT. Would be possible to patch the code so a server name is passed to the ClientHello step?

This would require the lookup table, a sort of /etc/hosts file for the Arduino.

Further reference:

• What is SNI? - Cloudflare
• TLS RFC
• SNI RFC
• MQTT Protocol Security introduction

  As a transport protocol, MQTT is concerned only with message transmission and it is the implementer’s responsibility to provide appropriate security features. This is commonly achieved by using TLS [RFC5246].

We could try issuing certificates for IP addresses, but unexpected issues may arise (is technically doable but very uncommon) especially due to how our infrastructure is built on AWS.

@manchoz any news on the PR?

Discussed changes have been implemented, thank you all for contributing!